doc: Add OpenSSL errors to API docs #34213

j3lamp · 2020-07-06T00:58:10Z

Checklist

documentation is changed or added
[ x] commit message follows commit guidelines

bnoordhuis

Generally LGTM, thanks.

I'm fairly sure UNABLE_TO_DECRYPT_CERT_SIGNATURE and UNABLE_TO_DECRYPT_CRL_SIGNATURE are never actually returned by openssl even though it lists them as error codes (and node handles them.)

I think HOSTNAME_MISMATCH is also one that's never going to show up because node handles host name verification itself, through tls.checkServerIdentity().

I'll leave it up to you if you want to leave them in or not.

bnoordhuis · 2020-07-06T19:00:49Z

doc/api/errors.md

+The certificate has expired: that is the notAfter date is before the current
+time.


Suggested change

The certificate has expired: that is the notAfter date is before the current

time.

The certificate has expired: the notAfter date is before the current time.

For consistency with the preceding description.

bnoordhuis · 2020-07-06T19:00:59Z

doc/api/errors.md

+<a id="CRL_NOT_YET_VALID"></a>
+#### `CRL_NOT_YET_VALID`
+
+The revocation data have a future issue date.


Suggested change

The revocation data have a future issue date.

The revocation data has a future issue date.

Technically "have" is more corerct. I'll reword it to avoid the awkwardness of "data".

bnoordhuis · 2020-07-06T19:02:24Z

doc/api/errors.md

+<a id="CERT_CHAIN_TOO_LONG"></a>
+#### `CERT_CHAIN_TOO_LONG`
+
+The certificate chain length is greater than the supplied maximum depth.


Suggested change

The certificate chain length is greater than the supplied maximum depth.

The certificate chain length is greater than the maximum depth.

"supplied" suggests it's configurable but it's not (in node - no binding for X509_STORE_CTX_set_depth().)

bnoordhuis · 2020-07-06T19:19:52Z

doc/api/errors.md

+
+The certificate’s signer was not a CA. This may happen if this was a version 1
+certificate, which is common with some CAs, or a version 3 certificate without
+the basic constrains extension.


Suggested change

the basic constrains extension.

the basic constraints extension.

I'd leave out the bit about v1 vs. v3 certificates. Openssl's docs say this, which IMO is generic yet complete enough:

a CA certificate is invalid. Either it is not a CA or its extensions are not consistent with the supplied purpose.

"The certificate’s signer" is a good clarification but I'm not 100% sure you can't also get it when you manipulate a CA certificate directly. In that case the error is about the certificate itself, not its signer.

Okay, I thought that GnuTLS' description was clear, but I am no expert. I'll switch it to OpenSSL's description. (Which is mostly what I used for the rest anyway.)
I'd be more embarrassed about "constrains" if it wasn't wrong in what I copied.

j3lamp · 2020-07-06T22:53:44Z

I left the three errors that shouldn't be seen simply to keep the list of what errors that node handles and the list that are documented the same. I made the rest of the changes you suggested. Thank you for the thorough review. Oh, I also rebased to avoid conflicts.

nodejs-github-bot · 2020-07-07T04:19:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32232/

PoojaDurgad · 2021-01-21T10:00:00Z

@j3lamp - need a rebase to resolve git conflicts

DerekNonGeneric · 2021-02-20T04:53:17Z

Would be nice to get this in. Seems good to go.

/cc @Trott

Trott · 2021-02-22T16:26:49Z

Non-blocking nit: I'd prefer the errors in each subsection be in alphabetical order unless there is a hugely compelling reason like some other obvious logical order. So, for example, CERT_HAS_EXPIRED would come before CERT_NOT_YET_VALID rather than after it.

Looks good to me. The C++ change is a comment only so running Jenkins may be superfluous. If anyone disagrees, we can run Jenkins once it comes out of lockdown (probably in a few hours).

Mifrill

Is it worth adding a link to a source like https://www.openssl.org/docs/ or any other authoritative sources to ensure accuracy, since this documentation is always good to verify the content against the official OpenSSL documentation?
Additionally, if there have been any recent updates or changes to these error codes, it's essential to reflect those changes in the documentation, since this PR is open for a while.

Fixes: nodejs#33705 PR-URL: nodejs#34213 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>

Fixes: #33705 PR-URL: #34213 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>

Fixes: nodejs#33705 PR-URL: nodejs#34213 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>

nodejs/node#34213

* chore: bump node in DEPS to v20.15.0 * doc: Add OpenSSL errors to API docs nodejs/node#34213 * test: crypto-rsa-dsa testing for dynamic openssl nodejs/node#52781 * src: allow preventing debug signal handler start nodejs/node#46681 * cli: allow running wasm in limited vmemory with --disable-wasm-trap-handler nodejs/node#52766 * chore: fixup indices --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]>

* chore: bump node in DEPS to v20.15.0 * doc: Add OpenSSL errors to API docs nodejs/node#34213 * test: crypto-rsa-dsa testing for dynamic openssl nodejs/node#52781 * src: allow preventing debug signal handler start nodejs/node#46681 * cli: allow running wasm in limited vmemory with --disable-wasm-trap-handler nodejs/node#52766 * chore: fixup patch indices * chore: update patches --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Jul 6, 2020

bnoordhuis approved these changes Jul 6, 2020

View reviewed changes

j3lamp force-pushed the feature/doc-ssl-errors branch from 4001550 to ff97f02 Compare July 6, 2020 22:51

danbev approved these changes Jul 7, 2020

View reviewed changes

j3lamp requested a review from a team August 10, 2020 16:06

fhinkel approved these changes Aug 24, 2020

View reviewed changes

j3lamp force-pushed the feature/doc-ssl-errors branch from ff97f02 to eedf9bf Compare January 27, 2021 02:38

Trott approved these changes Feb 22, 2021

View reviewed changes

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

Mifrill reviewed Nov 22, 2023

View reviewed changes

UlisesGascon approved these changes Nov 23, 2023

View reviewed changes

doc: add OpenSSL errors to API docs

e22bc1e

Fixes: nodejs#33705 PR-URL: nodejs#34213 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>

aduh95 force-pushed the feature/doc-ssl-errors branch from eedf9bf to e22bc1e Compare May 5, 2024 11:58

aduh95 merged commit e22bc1e into nodejs:main May 5, 2024
42 of 43 checks passed

targos mentioned this pull request May 13, 2024

v22.2.0 release proposal #52971

Merged

marco-ippolito mentioned this pull request Jun 17, 2024

v20.15.0 proposal #53486

Merged

codebytere added a commit to electron/electron that referenced this pull request Jun 24, 2024

doc: Add OpenSSL errors to API docs

98155c9

nodejs/node#34213

codebytere added a commit to electron/electron that referenced this pull request Jun 24, 2024

doc: Add OpenSSL errors to API docs

57b0cfb

nodejs/node#34213

codebytere added a commit to electron/electron that referenced this pull request Jun 24, 2024

doc: Add OpenSSL errors to API docs

64eadb8

nodejs/node#34213

codebytere added a commit to electron/electron that referenced this pull request Jun 24, 2024

doc: Add OpenSSL errors to API docs

fbc4e35

nodejs/node#34213

codebytere added a commit to electron/electron that referenced this pull request Jun 24, 2024

doc: Add OpenSSL errors to API docs

5617b58

nodejs/node#34213

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Add OpenSSL errors to API docs #34213

doc: Add OpenSSL errors to API docs #34213

j3lamp commented Jul 6, 2020

bnoordhuis left a comment

bnoordhuis Jul 6, 2020

bnoordhuis Jul 6, 2020

j3lamp Jul 6, 2020

bnoordhuis Jul 6, 2020

bnoordhuis Jul 6, 2020

j3lamp Jul 6, 2020

j3lamp commented Jul 6, 2020

nodejs-github-bot commented Jul 7, 2020

PoojaDurgad commented Jan 21, 2021

DerekNonGeneric commented Feb 20, 2021

Trott commented Feb 22, 2021

Mifrill left a comment

		The certificate has expired: that is the notAfter date is before the current
		time.

	The certificate has expired: that is the notAfter date is before the current
	time.
	The certificate has expired: the notAfter date is before the current time.

	The revocation data have a future issue date.
	The revocation data has a future issue date.

	The certificate chain length is greater than the supplied maximum depth.
	The certificate chain length is greater than the maximum depth.

	the basic constrains extension.
	the basic constraints extension.

doc: Add OpenSSL errors to API docs #34213

doc: Add OpenSSL errors to API docs #34213

Conversation

j3lamp commented Jul 6, 2020

Checklist

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Jul 6, 2020

Choose a reason for hiding this comment

bnoordhuis Jul 6, 2020

Choose a reason for hiding this comment

j3lamp Jul 6, 2020

Choose a reason for hiding this comment

bnoordhuis Jul 6, 2020

Choose a reason for hiding this comment

bnoordhuis Jul 6, 2020

Choose a reason for hiding this comment

j3lamp Jul 6, 2020

Choose a reason for hiding this comment

j3lamp commented Jul 6, 2020

nodejs-github-bot commented Jul 7, 2020

PoojaDurgad commented Jan 21, 2021

DerekNonGeneric commented Feb 20, 2021

Trott commented Feb 22, 2021

Mifrill left a comment

Choose a reason for hiding this comment